Suggestion: The var reference here is unnecessary. You're already dealing with builders is several places so I suggest just keeping it as a builder until you've applied all the customizations necessary:

val targetSymbol = toSymbol(targetShape)
    .toBuilder()
    .apply {
        if (nullableIndex.isMemberNullable(shape, NullableIndex.CheckMode.CLIENT)) boxed()

        shape.getTrait<DefaultTrait>()?.getDefaultValue(targetShape)?.let {
            defaultValue(it)
            if (it != "null") unboxed()
        }
    }
    .build()

Question: Looks like we box when the nullable index says to but then unbox if the default isn't null. Is that right? What happens if these two things conflict?

Style: Prefer when over if/else if/.../else.

+1

Question: Is the decimal check necessary? The f type specifier should mean that a decimal component is not required (i.e., 5f and 5.0f are identical).

Question: Should we be keeping the notion of boxed/unboxed in our codebase at this point? It seems like the nullability index should be preferred at this point, should we rename this or change the way we represent symbol nullability?

+1

Nit: This doesn't escape any quotes in the value. While unlikely, it's still better to be safe. Fortunately, we already have a String.dq() extension method for exactly this:

toNode().isStringNode -> toNode().toString().dq()

Style: I suggest a convenience extension property for getting the default type, similar to isNullable.

Question: Should version = "2" be the default?

so if a member has @default(null) in the model ("to explicitly indicate that the member has no default value or to override the default value requirement of a targeted shape."), this will be treated as having a MODELED default, even though semantically it makes the member nullable. So it would say nullable=true and defaultValueType=MODELED? I'm still trying to understand how MODELED is used, but just checking if this is ok/intended?

@default(null) will not make the member nullable, it will just override any default value which has been potentially set by a target shape. the only thing that makes members nullable is the nullability index provided by Smithy. My other response has an explanation of INFERRED vs. MODELED

Question: so all number shapes having a INFERRED default value means the shape knows what the value would be, but the shape can be nullable too, and it's the nullable check that'll decide how the code is generated, and if it is not nullable, only then that defaultValue is relevant? Trying to understand the motivation for maintaining INFERRED v/s MODELED.

The motivation behind INFERRED / MODELED is because we're using .defaultValue() in two ways now. The previous way is inferring that numbers and booleans have a default value (0, 0f, 0.0, false). The new way we use .defaultValue() is when it's modeled on a shape.

If a shape has an INFERRED default value but it's nullable, we set it to null. If a shape has a MODELED default value but it's nullable, we will use the default value.

I probably don't understand how this all fits, but why is isNullable not sufficient? Seems simpler if it ended up like that. What test case would break without the && check?

The reason for the && check is if something's nullable but it has a default value modeled, we want to write that default.

With && check:

public val foo: String? = "my modeled default value"

Without && check:

public val foo: String? = null // (discards the modeled default value)

I thought if something has a non-null default value, it won't be nullable. Atleast when it uses CLIENT mode, per https://github.com/awslabs/smithy/blob/b69aeb5dae6639b4381575005722bea02bb31ca8/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java#L142.

oh, but they can be nullable because of @clientOptional or @input and still have default value. got it.

seems useful to assert memberSymbol.isNullable to show the behavior.

Probably worth adding cases with @clientOptional, @addedDefault, @input?

And @required on nullable target shapes? like

structure MyStruct {
    @required
    quux: Integer,
}

Given kotlin SDK is not GA, could NullableIndex.CheckMode.CLIENT be used?

And note, nullableIndex.isMemberNullable(shape) uses CheckMode.CLIENT.

We're stuck using CLIENT_ZERO_VALUE_V1_NO_INPUT until S3 makes all their numbers and booleans nullable. We tried changing it in this PR but faced some issues with @default and nullability on things like PutObjectRequest's bucketKeyEnabled.

Internal reference: P83747977

We can hold off on merging this PR until it's resolved internally, and then I can update to use CheckMode.CLIENT

got it. if this needs to be merged sooner for some reason, as long as there's tracking / TODO somewhere to change to CLIENT that sounds fine.

I spoke with the team, we'll merge the PR now and add a tracking item to update to CLIENT